incubator/kube-downscaler - Expand configuration options for service account #23058

patrungel · 2020-07-02T20:42:54Z

Is this a new chart

No

What this PR does / why we need it:

The present introduces support for imagePullSecrets, annotations, and extra labels for ServiceAccount in use

I moved SA options out of RBAC tree to decouple SA configuration from the one for roles.

I took the liberty of incrementing minor version of the chart as the change is a feature increment.

Special notes for your reviewer:

None

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md
Title of the PR starts with chart name (e.g. [stable/mychartname])

k8s-ci-robot · 2020-07-02T20:43:05Z

Hi @patrungel. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

patrungel · 2020-07-02T20:49:12Z

/assign @Pluies

Pluies · 2020-07-05T19:49:04Z

Hi @patrungel!

This is a breaking change by removing rbac.serviceAccountName, so it'd need to be a major version change if we go this route.

Looking at other popular charts (like cluster-autoscaler, coredns or prometheus-operator), it looks like the usual approach to do that would be with:

serviceaccount.create controls whether a new serviceAccount will be created; sometimes just rbac.create
serviceaccount.name can be set to an existing serviceAccount name, created outside the chart for full manual control
^ These two appear pretty standard across charts, and I think (if we do this :) ) we should follow this unwritten convention.
serviceAccount.annotations, sometimes serviceAccountAnnotations to add annotations on the newly-created serviceAccount

But I haven't seen any other chart that define either labels or pull secrets for automatically-created serviceaccounts. I'd argue that if someone needs that level of customisation, they should probably create the serviceAccount separately and define it with serviceAccount.name 👍

What do you think?

cc @Rowern

patrungel · 2020-07-06T10:37:20Z

Thanks for the review, @Pluies .

Agree on the breaking character of the change. Is it okay ot up major version, considering the chart is in pre-release (major version 0)? https://semver.org/#how-do-i-know-when-to-release-100
Confirmed the more adopted layout for serviceAccount to be

create: true
  # The name of the ServiceAccount to use.
  # If not set and create is true, a name is generated using the fullname template
  # name:

I'll comply with the above.

I'd favour serviceAccount.annotations over serviceAccountAnnotations since we have other serviceAccount.* variables.
Confirmed: annotations are rare, labels are virtually non-existent (spotted just once really) . I'll drop labels, and leave annotations.
imagepullsecrets is widely supported feature (83 charts seem to have it), but rather as a top-level variable. I can move the variable up the structure and add image pull secrets to the ServiceAccount if created, otherwise to the Deployment. wdyt, @Pluies ?

UPD: all items but the version are now committed.

Pluies

Agree on the breaking character of the change. Is it okay ot up major version, considering the chart is in pre-release (major version 0)? https://semver.org/#how-do-i-know-when-to-release-100

I guess that makes sense, especially because we're in the incubator part of the charts :)

The imagePullSecrets part looks fine. 👍

Just a couple of notes on this PR and this is mergeable!

Pluies · 2020-07-09T17:47:15Z

incubator/kube-downscaler/README.md

+| `resources`                  | Downscaler pod resource requests & limits                                                               | `{}`                          |
+| `securityContext`            | SecurityContext to apply to the downscaler pod                                                          | `{}`                          |
+| `rbac.create`                | If true, create & use RBAC resources                                                                    | `true`                        |
+| `serviceAccount.create`      | If true, create & use a ServiceAccount                                                                  | `""`                          |


The default here should be true

Fixed that one, thanks!

Pluies · 2020-07-09T17:49:50Z

incubator/kube-downscaler/templates/serviceaccount.yaml

 apiVersion: v1
 kind: ServiceAccount
 metadata:
-  name: {{ include "kube-downscaler.fullname" . }}
+  name: {{ include "kube-downscaler.serviceAccountName" . }}
+  namespace: {{ .Release.Namespace }}


Is this line needed? 🤔 I would think it creates resources in the release namespace by default.

That's an ongoing discussion for about a year or so: helm/helm#5465
While in general helm is supposed to place items into the correct namespace, there are use-cases depending on namespace field to be present. I err on the side of caution here.

🤷 fine; probably should be added to other resources for consistency, but I won't hold the PR based on this!

patrungel · 2020-07-09T19:15:48Z

I guess that makes sense, especially because we're in the incubator part of the charts :)

Just for the sake of clarity, is it going to be 0.5.0 or 1.0.0?

Pluies · 2020-07-10T16:47:09Z

@patrungel let's keep 0.5.0 👍

/ok-to-test

/approve

k8s-ci-robot · 2020-07-10T16:47:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrungel, Pluies

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~incubator/kube-downscaler/OWNERS~~ [Pluies]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stale · 2020-08-09T17:09:34Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

scottrigby · 2020-08-21T13:47:12Z

Edit: deleted comment

scottrigby · 2020-09-09T21:03:04Z

Edit: deleted comment

Pluies · 2020-09-10T09:58:52Z

@scottrigby uh, this PR is for the kube-downscaler, as far as I know it has nothing to do with Prometheus – did you actually mean to close it or was it a mistake?

scottrigby · 2020-09-10T11:05:49Z

@Pluies you're right 👌 This was my mistake somehow from the previous comment, then last night my search picked it up again. Thanks 🙂

The present introduces support for imagePullSecrets and ServiceAccount annotations Signed-off-by: Danil Mironov <[email protected]>

stale · 2020-10-12T18:30:55Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

stale · 2020-10-31T04:39:54Z

This issue is being automatically closed due to inactivity.

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 2, 2020

k8s-ci-robot requested review from Pluies and Rowern July 2, 2020 20:43

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 2, 2020

k8s-ci-robot assigned Pluies Jul 2, 2020

patrungel force-pushed the kube-downscaler branch 2 times, most recently from e58e014 to becfc21 Compare July 7, 2020 08:52

Pluies suggested changes Jul 9, 2020

View reviewed changes

patrungel force-pushed the kube-downscaler branch from becfc21 to c80cae9 Compare July 9, 2020 19:19

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 10, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2020

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 9, 2020

stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 21, 2020

scottrigby closed this Sep 9, 2020

scottrigby reopened this Sep 10, 2020

Expand configuration options for service account

08f6f24

The present introduces support for imagePullSecrets and ServiceAccount annotations Signed-off-by: Danil Mironov <[email protected]>

patrungel force-pushed the kube-downscaler branch from c80cae9 to 08f6f24 Compare September 12, 2020 17:23

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 12, 2020

stale bot closed this Oct 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

incubator/kube-downscaler - Expand configuration options for service account #23058

incubator/kube-downscaler - Expand configuration options for service account #23058

patrungel commented Jul 2, 2020

k8s-ci-robot commented Jul 2, 2020

patrungel commented Jul 2, 2020

Pluies commented Jul 5, 2020 •

edited

Loading

patrungel commented Jul 6, 2020 •

edited

Loading

Pluies left a comment

Pluies Jul 9, 2020

patrungel Jul 9, 2020

Pluies Jul 9, 2020

patrungel Jul 9, 2020

Pluies Jul 10, 2020

patrungel commented Jul 9, 2020

Pluies commented Jul 10, 2020

k8s-ci-robot commented Jul 10, 2020

stale bot commented Aug 9, 2020

scottrigby commented Aug 21, 2020 •

edited

Loading

scottrigby commented Sep 9, 2020 •

edited

Loading

Pluies commented Sep 10, 2020

scottrigby commented Sep 10, 2020

stale bot commented Oct 12, 2020

stale bot commented Oct 31, 2020

incubator/kube-downscaler - Expand configuration options for service account #23058

incubator/kube-downscaler - Expand configuration options for service account #23058

Conversation

patrungel commented Jul 2, 2020

Is this a new chart

What this PR does / why we need it:

Special notes for your reviewer:

Checklist

k8s-ci-robot commented Jul 2, 2020

patrungel commented Jul 2, 2020

Pluies commented Jul 5, 2020 • edited Loading

patrungel commented Jul 6, 2020 • edited Loading

Pluies left a comment

Choose a reason for hiding this comment

Pluies Jul 9, 2020

Choose a reason for hiding this comment

patrungel Jul 9, 2020

Choose a reason for hiding this comment

Pluies Jul 9, 2020

Choose a reason for hiding this comment

patrungel Jul 9, 2020

Choose a reason for hiding this comment

Pluies Jul 10, 2020

Choose a reason for hiding this comment

patrungel commented Jul 9, 2020

Pluies commented Jul 10, 2020

k8s-ci-robot commented Jul 10, 2020

stale bot commented Aug 9, 2020

scottrigby commented Aug 21, 2020 • edited Loading

scottrigby commented Sep 9, 2020 • edited Loading

Pluies commented Sep 10, 2020

scottrigby commented Sep 10, 2020

stale bot commented Oct 12, 2020

stale bot commented Oct 31, 2020

Pluies commented Jul 5, 2020 •

edited

Loading

patrungel commented Jul 6, 2020 •

edited

Loading

scottrigby commented Aug 21, 2020 •

edited

Loading

scottrigby commented Sep 9, 2020 •

edited

Loading